Memorized DOMAIN ObjectType for a proper export#655
Memorized DOMAIN ObjectType for a proper export#655FedericoSpada wants to merge 1 commit intocanopen-python:masterfrom
Conversation
Changed to save whether a Variable has ObjectType==DOMAIN while importing an EDS, so that it is possible to export it unchanged.
bizfsc
left a comment
bizfsc
left a comment
There was a problem hiding this comment.
Looking good. As far as I can check it, no new problems introduced for mypy.
friederschueler
left a comment
friederschueler
left a comment
There was a problem hiding this comment.
Good fix for a real round-trip data-loss bug. The overall approach — adding is_domain on import and using it on export — is clean and well-tested.
A few small points:
- Two places in
eds.pyhave missing spaces around==inside a keyword argument (see inline comments). - The new boolean assertions in the tests would be more idiomatic as
assertFalse/assertTrue. - Worth considering storing the full
object_typeinteger instead of just a boolean, but that is a larger API change and not required for this fix.
|
|
||
| if object_type in (objectcodes.VAR, objectcodes.DOMAIN): | ||
| var = build_variable(eds, section, node_id, index) | ||
| var = build_variable(eds, section, node_id, index, is_domain=object_type==objectcodes.DOMAIN) |
There was a problem hiding this comment.
PEP 8: missing spaces around the == comparison inside the keyword argument.
| var = build_variable(eds, section, node_id, index, is_domain=object_type==objectcodes.DOMAIN) | |
| var = build_variable(eds, section, node_id, index, is_domain=object_type == objectcodes.DOMAIN) |
| object_type = int(eds.get(section, "ObjectType"), 0) | ||
| except NoOptionError: | ||
| object_type = objectcodes.VAR | ||
| var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type==objectcodes.DOMAIN) |
There was a problem hiding this comment.
Same PEP 8 spacing issue as above.
| var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type==objectcodes.DOMAIN) | |
| var = build_variable(eds, section, node_id, index, subindex, is_domain=object_type == objectcodes.DOMAIN) |
| self.assertEqual(var.name, 'Producer heartbeat time') | ||
| self.assertEqual(var.data_type, canopen.objectdictionary.UNSIGNED16) | ||
| self.assertEqual(var.access_type, 'rw') | ||
| self.assertEqual(var.is_domain, False) |
There was a problem hiding this comment.
Nit: for boolean checks the unittest idiom is assertFalse/assertTrue rather than assertEqual(..., False/True). Same pattern applies to the other four analogous assertions added in this PR.
| self.assertEqual(var.is_domain, False) | |
| self.assertFalse(var.is_domain) |
| #: Access type, should be "rw", "ro", "wo", or "const" | ||
| self.access_type: str = "rw" | ||
| #: The variable represents a DOMAIN ObjectType | ||
| self.is_domain: bool = False |
There was a problem hiding this comment.
Design consideration: a boolean is_domain only captures one special ObjectType. Storing the full object_type: int attribute (defaulting to objectcodes.VAR) would be more flexible (e.g. NULL, DEFTYPE) and avoids losing that information for manually constructed ODVariable instances. Just something to think about — a boolean is perfectly fine for the stated use-case.
3 participants
While working with an EDS that had DOMAIN objects, I've noted that when imported and exported using this library, all DOMAIN Objects were converted to normal Variables.
While importing an EDS, I've changed to save whether a Variable was created from an object with
ObjectType==DOMAIN. In this way, it is possible to export it unchanged with the proper ObjectType value.